Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add resolveDid #6

Merged
merged 32 commits into from
Jan 3, 2024
Merged

add resolveDid #6

merged 32 commits into from
Jan 3, 2024

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Dec 12, 2023

closes #4

  • Add DID types (DIDDocument) and DID parsing (DIDurl)
  • resolve did history with the registry
  • wrap did document
  • tests
  • update docs

Not implementing for this PR

  • blockTag for DID

related to roadmap#23

Example

Run against Sepolia with RUST_LOG=didethresolver=debug ./target/release/didethresolver -p "wss://ethereum-sepolia.publicnode.com"

example query:
curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "did_resolveDid", "params": ["0x423B6F365C14F4b233928088FF937e01c143A328"]}' http://localhost:9944

Pretty print the result:
curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "did_resolveDid", "params": ["0x423B6F365C14F4b233928088FF937e01c143A328"]}' http://localhost:9944 | jq .result

@@ -0,0 +1,223 @@
//! Convenience Wrapper around [`Url`] for DID URIs according to the [DID Spec](https://www.w3.org/TR/did-core/#did-syntax)
Copy link
Contributor Author

@insipx insipx Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well but the url crate is based on the whatwg standard

Ideally we find a crate that already implemented the standard DID is a derivative of, or we can just use regex like did-ethr reference implementation. It seems to me that the url crate is more flexible than regex, though, and we get queries + fragments for free. We just need to impl id parsing ourselves (separate network id and address) which isn't a big ask, and could use regex for.

@insipx insipx changed the title add resolveDID functionality add resolveDid Dec 12, 2023
Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a pretty good start! I hit a few snags because the tests dont appear to build and there are a few warnings outstanding.

This project may be helpful: https://github.com/OwnYourData/didlint

It appears there is a network api as well.

@insipx
Copy link
Contributor Author

insipx commented Dec 13, 2023

This looks like a pretty good start! I hit a few snags because the tests dont appear to build and there are a few warnings outstanding.

This project may be helpful: https://github.com/OwnYourData/didlint

It appears there is a network api as well.

thanks! Definitely cleanup to do and still working on generating the actual did document from the did history. There is a network endpoint, did_resolveDid that can be called:

curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "did_resolveDid", "params": ["0x6CEb0bF1f28ca4165d5C0A04f61DC733987eD6ad"]}' http://localhost:9944/ | jq .result

After starting with cargo run --release -- p "wss://ethereum-spolia.publicnode.com" (need to change to env variables)

but currently only returns a stub did document until wrap_did_document is finished, although it does log the event history if you run the server with RUST_LOG=didethresolver=debug.

The tests don't build because i haven't updated the with_client harness yet. It requires starting a eth server now which can be done in-code with Foundry, but maybe should be moved to integration tests. While I get the first iteration of wrap_did_document finished i'm just testing against the already-deployed sepolia contract and server

I'll also make sure to test everything with did-lint. In the future, it would be nice to have did-lint run automatically on CI push against Sepolia to verify our documents

@insipx insipx linked an issue Dec 20, 2023 that may be closed by this pull request
@insipx insipx self-assigned this Dec 20, 2023
@insipx insipx marked this pull request as ready for review December 20, 2023 21:53
@insipx
Copy link
Contributor Author

insipx commented Dec 20, 2023

This could use some more tests, but overall I feel like it's in a pretty good place. I'll create a ticket for the cases I'd like to create tests for

use didethresolver::run;

#[tokio::main]
async fn main() -> Result<()> {
Copy link
Contributor Author

@insipx insipx Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This separation of a main and lib is done to make integration testing possible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2023

Codecov Report

Attention: 97 lines in your changes are missing coverage. Please review.

Comparison is base (ed3cedb) 91.37% compared to head (19d0af0) 91.62%.

Files Patch % Lines
src/types/ethr.rs 95.00% 25 Missing ⚠️
src/lib.rs 0.00% 15 Missing ⚠️
src/types.rs 87.50% 15 Missing ⚠️
src/types/did_url.rs 91.30% 12 Missing ⚠️
src/rpc/methods.rs 54.54% 10 Missing ⚠️
src/util.rs 40.00% 6 Missing ⚠️
src/types/did_parser.rs 98.10% 5 Missing ⚠️
src/resolver/did_registry.rs 80.00% 4 Missing ⚠️
src/main.rs 0.00% 3 Missing ⚠️
src/resolver.rs 97.50% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   91.37%   91.62%   +0.24%     
==========================================
  Files           5       11       +6     
  Lines          58     1182    +1124     
==========================================
+ Hits           53     1083    +1030     
- Misses          5       99      +94     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding tests and documentation! This is great work and a great start. Please add a ticket to support handling for revoked attributes as that is somewhat important

@@ -0,0 +1,2 @@
ADDRESS="127.0.0.1:9944"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these quotes are not required in this context. I believe in the TypeScript dotenv these quotes will be included in the environment, not sure how rust behaves but its worth checking

// so it could be worth defining a config file that maps chainId to RPC provider (like
// did-eth-resolver)
/// The address of the DID Registry contract on the Ethereum Sepolia Testnet
pub const DID_ETH_REGISTRY: &str = "0xd1D374DDE031075157fDb64536eF5cC13Ae75000";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a proxy contract with a stable address on all networks. Allowing override probably makes sense, but may also create a security loophole.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be explicit that the override is only for tests, or restrict overriding the tests only when we're sure we're in a test environment

use didethresolver::run;

#[tokio::main]
async fn main() -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Ok(self.wrap_did_document(public_key, history).await?)
}

async fn changelog(&self, public_key: H160) -> Result<Vec<(DIDRegistryEvents, LogMeta)>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we handing revoked an expired events? If that is not part of this change please make sure we open an issue on that before this gets merged.

Copy link
Contributor Author

@insipx insipx Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should handle these events in the wrap_did_document method, although we should definitely make sure of it and add tests specifically for revoked attributes, added a ticket here

@insipx insipx merged commit aa7b8d7 into main Jan 3, 2024
3 checks passed
@37ng 37ng deleted the insipx/resolveDid branch January 5, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Endpoint: 'resolveDid'
5 participants